-
-
Notifications
You must be signed in to change notification settings - Fork 343
Add refresh_attributes() and implement cache_attrs for Group and Array #3215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add refresh_attributes() and implement cache_attrs for Group and Array #3215
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3215 +/- ##
==========================================
+ Coverage 94.73% 94.75% +0.01%
==========================================
Files 78 78
Lines 8646 8670 +24
==========================================
+ Hits 8191 8215 +24
Misses 455 455
🚀 New features to boost your workflow:
|
cache_attrs : bool, optional | ||
If True (default), user attributes will be cached for attribute read | ||
operations. If False, user attributes are reloaded from the store prior | ||
to all attribute read operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this docstring says true is the default, but the parameter itself has a default of None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other places defining a cache_attrs
argument did the same, so I decided to follow suit. (:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets break with the past and ensure that the docstring matches the annotation here
anyone have opinions on whether cache_attrs should have a default of |
Can someone write up a detailed proposal of the model here? The interaction between in-memory metadata objects and their serialized counterparts is a big topic. If we're formally committing to something then we should do so carefully, and document it extensively. |
From my perspective of "just solving the issue", the crux of this PR is the As for a model... that would be a good idea, I agree. If I understand the current main branch code correctly (from a cursory reading, mind it), the current model for caching data is that the store may cache data, but Array/Group never cache data themselves, only metadata. Metadata is cached when opening an array/group, and kept cached until you discard the respective object. This PR adds the possibility of manually (or automatically) reloading just the attributes of an array or group. Ideally, there would be support for reloading the whole metadata in case there might have been changes made by others—but for that, we would need to either make arrays/groups unfrozen, or guide users to re-open the whole array/group (and replace all references to the old version) in case there have been changes made by another process. As for a better model:
Thoughts? |
Should resolve #3178, if one passes
cache_attrs=False
when creating the various groups.TODO:
docs/user-guide/*.rst
changes/